-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Divorce Redis session and cookie expiration #1130
Conversation
There may be a simpler solution here. @jgrevich did some digging already and if we remove the expiry of the cookie, it becomes a session cookie and is deleted when the browser is closed.
|
We discussed this very same issue during the ICAM project but it wasn't deemed serious enough to address and has some consequences to think about. I'm pasting @amoose's comment from the ICAM issue:
|
Yes, thank you for bringing this up, Moncef. This cookie expiry does not address session expiry in a SP but it does ensure that login.gov won't preserve a session when the browser is closed and reopened. |
upstream PR here roidrage/redis-session-store#84 The issue is that |
Note that the session will only be preserved if less than 8 minutes have passed between the closing and reopening of the browser AND a new request is made. If the tab wasn't closed, re-opening the browser doesn't make a new web request. Are we saying 8 minutes is too much of a risk? Would lowering the |
@monfresh thanks for digging that up. I tested locally and by removing the expiration date does not affect multiple tabs or SP sessions. It only affects the IdP session when the browser is closed:
It looks like we may have to do a bit more to make sure session data is wiped from redis as well. |
@jgrevich when you remove the See #1130 (comment) |
Here's how I understand the problem. Because the Rails session cookie has an explicit expiration value, most browsers will persist the cookie until that expiration time. That means that exiting the browser (quitting) does not delete the session cookie. The simple solution would be to not set an explicit expiration value on the cookie, so that the cookie is destroyed when the browser exists. That's straightforward. What's problematic is that Rails session code for the Redis driver (and others) does not distinguish between the expire value for the server-side storage and the expire value for the browser-side cookie. It uses the same value for both. So if we remove the expiration value to fix the cookie persistence, we also stop automatically expiring the session data on the server side. This PR addresses that problem, by splitting the configuration into 2 different settings: one for the cookie expiration ( |
New PR 18F/redis-session-store#1 |
**Why**: We want the cookie to expire if/when the browser is closed, but the Redis server-side expiration to expire after the configured number of minutes.
850356e
to
09f041c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
**Why**: We want the cookie to expire if/when the browser is closed, but the Redis server-side expiration to expire after the configured number of minutes.
**Why**: We want the cookie to expire if/when the browser is closed, but the Redis server-side expiration to expire after the configured number of minutes.
Why: We want the cookie to expire if/when the browser is closed,
but the Redis server-side expiration to expire after the configured
number of minutes.